-
Notifications
You must be signed in to change notification settings - Fork 2.6k
feat: add custom region input support for AWS Bedrock provider #5925
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- Add awsCustomRegion field to ProviderSettings schema - Update BEDROCK_REGIONS to include "Custom region..." option - Modify Bedrock UI component to support custom region input - Add conditional UI similar to VPC endpoint pattern - Handle state management for switching between standard and custom regions - Preserve existing validation logic and maintain backward compatibility - Add comprehensive tests for custom region functionality Fixes #5923
|
@roomote-agent Address this: Critical Issue: Missing Backend Implementation The PR adds a custom region input feature but the backend doesn't actually use the awsCustomRegion field. When "custom" is selected and a user enters "us-west-3", the backend still receives awsRegion: "custom" instead of the actual region value. Other Findings: Missing i18n translations - The placeholders and example text aren't properly internationalized Commit your changes to this PR, do not create a new one. |
|
Hi @daniel-lxs, I see your feedback. I'm working on addressing all the issues you've identified:
I'll commit the fixes to this PR shortly. |
|
@roomote-agent Use the switch_mode tool to switch to pr-fixer mode and fix the failing unit test. |
|
I see the failing unit tests. Let me switch to pr-fixer mode and investigate the issue. |
- Backend now properly uses awsCustomRegion when awsRegion is 'custom' - Added i18n translations for custom region UI elements - Added validation for AWS region format (e.g., us-west-3) - Improved state management to preserve custom region value when switching - Added comprehensive tests for custom region functionality
|
Hi @daniel-lxs, I've successfully addressed all the issues you identified: ✅ Fixed Issues1. Critical Issue: Backend ImplementationThe backend now properly uses the Changes made:
2. i18n TranslationsAdded proper internationalization for all custom region UI elements:
3. ValidationImplemented comprehensive validation for custom region format:
4. State ManagementImproved the UX by preserving the custom region value when switching between regions:
5. Test CoverageAdded comprehensive tests:
Summary of Changes
The implementation ensures backward compatibility while providing the flexibility users need to access new AWS regions immediately. |
| this.options = options | ||
| let region = this.options.awsRegion | ||
| // Use custom region if awsRegion is "custom" | ||
| let region = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The region resolution logic (using awsRegion === 'custom' && awsCustomRegion) is duplicated (also at line 952). Consider extracting it into a helper function for clarity and consistency.
| Select: ({ children, onValueChange, value }: any) => { | ||
| // Store the onValueChange callback on the window for testing (for new tests) | ||
| if (typeof window !== "undefined") { | ||
| ;(window as any).__selectOnValueChange = onValueChange |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Select mock stores the onValueChange callback in both window.__selectOnValueChange and mockSelectCallbacks; consider using a single consistent mechanism to avoid confusion (e.g. in tests, some use window while others use mockSelectCallbacks).
|
I see the request to fix the failing unit test. I will switch to pr-fixer mode and investigate the issue now. |
|
Closing as the regions are fairly easy to add and this solution seems overcomplicated |
|
Hi @daniel-lxs, I see you requested me to switch to pr-fixer mode to fix the failing unit test. However, I notice that this PR has already been closed with the comment "Closing as the regions are fairly easy to add and this solution seems overcomplicated". Since the PR is closed, I cannot make any further changes to it. If you would like me to help with a different approach or create a new PR with a simpler solution, please let me know! |
This PR implements custom region input support for AWS Bedrock provider as requested in issue #5923.
Changes Made
1. Schema Updates
awsCustomRegionfield to thebedrockSchemainProviderSettings2. Region Options Enhancement
BEDROCK_REGIONSarray to include a "Custom region..." option at the end3. UI Component Updates
customRegionSelectedsimilar to existing VPC endpoint pattern4. Comprehensive Testing
Key Features
✅ Non-breaking change - Maintains backward compatibility with existing configurations
✅ Immediate access - Users can access newly launched AWS regions without waiting for code updates
✅ Intuitive UX - Follows established patterns (similar to VPC endpoint functionality)
✅ Proper validation - Preserves existing validation logic and error handling
✅ State management - Handles switching between standard and custom regions seamlessly
Use Case Example
When AWS launches a new region like
us-west-3:us-west-3manuallyTesting
Fixes #5923
Important
Add support for custom AWS regions in Bedrock provider, including schema, UI, and backend updates, with comprehensive testing.
awsCustomRegiontobedrockSchemainprovider-settings.tsto store custom region identifier.BEDROCK_REGIONSinbedrock.tsto include "Custom region..." option and sort it to the end.Bedrock.tsxto handle custom region input with validation and state management.AwsBedrockHandlerinbedrock.tsto useawsCustomRegionwhenawsRegionis "custom".bedrock-custom-region.spec.tsfor testing custom region functionality.settings.jsonfor custom region validation messages.This description was created by
for 4a56357. You can customize this summary. It will automatically update as commits are pushed.